Migrate useDeferredValue and useTransition #17058
Conversation
…an/react into use_suspense_deferred_val
lunaruan
changed the title
useSuspenseDeferredValue and useSuspenseTransition
|
React: size: 0.0%, gzip: 0.0% ReactDOM: size: 🔺+0.5%, gzip: 🔺+0.3% Details of bundled changes.Comparing: abedf17...7b7db2d react
react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
react-debug-tools
|
|
As part of this migration I intended to drop the “Suspense” part. Let’s just call them useDeferredValue and useTransition. The Suspense part was to bring them into a brand but going forward, it’s just going to be annoying to have the longer name. It’s also a misnomer since given this set up, this is the primary API to call into the Concurrent Mode APIs even if you don’t use Suspense at all. |
lunaruan
changed the title
|
You can probably update this warning now. react/packages/react-reconciler/src/ReactFiberWorkLoop.js Lines 2931 to 2933 in 5a71cbe |
|
We'll also need an accompanying PR to update the hooks API docs: |
|
|
||
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | ||
| ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; | ||
| ReactFeatureFlags.enableSchedulerTracing = true; | ||
| ReactFeatureFlags.flushSuspenseFallbacksInTests = false; |
There was a problem hiding this comment.
Why is this necessary? Can we rewrite the test somehow? I can pair with you on this if needed
| @@ -27,6 +27,7 @@ describe('ReactHooks', () => { | |||
|
|
|||
| ReactFeatureFlags = require('shared/ReactFeatureFlags'); | |||
| ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; | |||
| ReactFeatureFlags.enableStableConcurrentModeAPIs = true; | |||
There was a problem hiding this comment.
If you rebase on master, you can wrap the concurrent tests with an __EXPERIMENTAL__ check instead.
| 'A', | ||
| 'B', | ||
| 'Suspend! [B]', | ||
| 'Loading', |
There was a problem hiding this comment.
I... don't understand the purpose of this (why 'A' is in the yields). deferredText will finish the current commit with the old value, and then update itself in a separate commit, but why? Doesn't <Suspense> already do that "for free"?
There was a problem hiding this comment.
The yields are not important. The output is important. If it was using Suspense, the output would be [span('B'), span('Loading')] which isn't desirable here.
There was a problem hiding this comment.
I'd expect Suspense to hold off on committing the subtree at all (so it'd stay showing span('A')) until the expirationTime is reached.
...though I guess act() forces an expirationTime flush.
There was a problem hiding this comment.
In that case we couldn't commit the outer A so it would be [span('A'), span('A')]. We have another way to do that which is what useTransition is for.
We never commit anything in an inconsistent state by default. So either we commit A in both slots or B in both slots. Early explorations included ways of leaving some trees inconsistent but the downstream effects on that in a reactive system was too difficult to reason about. E.g. imaging you also switched to Dark Mode in a Context at the same time.
useDeferredValue is a limited form of explicitly opting in to inconsistency for a particular value while keeping other values consistent.
| ReactCurrentBatchConfig.suspense = config === undefined ? null : config; | ||
| try { | ||
| setPending(false); | ||
| callback(); |
There was a problem hiding this comment.
Does the callback run as a potentially separate commit, or does being inside Scheduler.next prevent non-explicit "unbatching"?
There was a problem hiding this comment.
The intention here is to:
- Set pending false at high priority
- Batch setting pending to false and doing the actual state change at lower priority
As a result, you'll see pending immediately turn true, but then it turn false together with the actual state change you wanted to do.
| }, | ||
| [config, isPending], | ||
| ); | ||
| return [startTransition, isPending]; |
There was a problem hiding this comment.
This is the only built-in hook producing a pair which returns a function as the first, instead of the second element. This will take some education.
There was a problem hiding this comment.
That does seem odd. Should it be flipped?
There was a problem hiding this comment.
The first argument is useful without the second but not the inverse.
There was a problem hiding this comment.
Shouldn’t we nudge you to use the busy return value tho?
| } finally { | ||
| ReactCurrentBatchConfig.suspense = previousConfig; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Why do we not pass the TimeoutConfig to Scheduler.unstable_next in the mount case, when we do in the update case?
There was a problem hiding this comment.
Nice catch. unstable_next doesn't accept a second argument. It must have been a copypasta from withSuspenseConfig.
There was a problem hiding this comment.
Yeah, it was accidental copy/paste. Deleted it! :)
…uspense_deferred_val
| } finally { | ||
| ReactCurrentBatchConfig.suspense = previousConfig; | ||
| } | ||
| }, config); |
There was a problem hiding this comment.
This config doesn't seem right. Probably a copy paste error.
|
I wonder if we should remove If we do, then the question becomes whether we should move ReactCurrentBatchConfig into the renderer since it can now be local to a component which simplifies things a bit. Especially versioning but limits our ability to move it to become cross-renderer in the future. |
|
Are these being planned as hook versions for something like |
|
No (these are different things) |
Migrated
useDeferredValueanduseTransitionfrom Facebook's www repo intoReactFiberHooks.